fix a bunch of tests that were failing #1295

jeffshaver · 2019-02-24T23:09:16Z

@ljharb I went took a look at the tests that were failing. seemed like all but one of them existed because there were exports for things that didn't exist, i.e. export { bar}, when bar isn't defined.

There is one test currently failing. However, after reading it, I think it should be deleted because what it says isn't true. The test reads: should return 'internal' for module from 'node_modules' if 'node_modules' missed in 'external-module-folders'. However, the check for isExternalPath checks whether or not the path` argument is falsy. If it is falsy, or if it is found in the import/external-module-folders, then it returns true. In the case of that test, the path is undefined (because that file doesn't exist), and therefore the test is failing.

coveralls · 2019-02-24T23:26:23Z

Coverage increased (+3.2%) to 97.313% when pulling 158cd80 on jeffshaver:fix-tests into bdc05aa on benmosher:master.

ljharb · 2019-02-25T08:49:26Z

I'm a bit confused; there are two tests in that part of the file - one with node_modules present in external-module-folders, one without.

I think we need to keep both tests, but it's entirely possible that they need tweaking to test the proper thing. (the rest of these changes look good)

jeffshaver · 2019-02-25T12:13:17Z

Sorry. Here is the test I'm talking about: https://github.com/benmosher/eslint-plugin-import/blob/master/tests/src/core/importType.js#L96

jeffshaver · 2019-02-25T12:30:28Z

If we need that test, I'm not sure what to change to make it work. If you pull out the path check in isExternalPath, then I have a feeling other tests will break (not tested).

jeffshaver · 2019-02-25T12:32:24Z

confirmed that removing the !path check in isExternalPath in importType.js breaks 88 tests.

jeffshaver · 2019-02-26T22:59:21Z

@ljharb just pinging to see what I should do with that last test

ljharb · 2019-02-26T23:13:51Z

I'm not really certain. When node_modules is not included in that list, it seems like nothing could be considered external, since all node_modules things would be considered "internal". Is that not possible to ensure?

jeffshaver · 2019-02-26T23:19:33Z

I agree with the extent comment. But how can we assume it's internal if it can't be resolved by the resolver?

ljharb · 2019-02-26T23:21:22Z

Are you suggesting this test was never good to have in the first place?

jeffshaver · 2019-02-26T23:40:09Z

@ljharb i dont think the test is good. Honestly, it feels like if the resolver can't resolve the path, then it should be unknown. But I imagine that would have to be a breaking change. Just removing that one check from the isExternalPath breaks 88 tests. So I can't imagine that we could change it and fix all the breaking tests without having a breaking change on our hands. So I guess I am wondering if we should remove the one failing test (because we can't make it pass currently), merge this, merge my other pr and then maybe there is a way forward in making that use case make more sense?

One issue, I guess, is we dont know how long that test has been broken. But it seems like it wouldn't be viable to make that work without upping to a new major version. And if you are going to do that, then that test case seems like it should return 'unknown' and not 'internal' anyway

ljharb · 2019-02-27T00:01:27Z

We definitely aren't going to do a major version at this time.

Since removing that check breaks a bunch of tests, it's clearly needed. If we want to remove or change this pair of tests, that might be fine, but i'd like to have a really good understanding of why the tests existed in the first place (since presumably they passed at one time).

jeffshaver · 2019-02-27T00:21:39Z

Yeah. I mean, i can comment it out, remove it, or whatever we want.

I am going to try to dig and see if I can figure out when it broke.

jeffshaver · 2019-02-27T01:17:03Z

@ljharb i found the commit that started breaking that test: fb8e1e5

ljharb · 2019-02-27T04:48:22Z

@jeffshaver woof, ok. I wonder what that would have caused, since the only difference between builtinModules and resolve/lib/core would be that resolve has more a accurate list of core modules.

jeffshaver · 2019-02-27T12:22:31Z

@ljharb What do you think is the path forward?

ljharb · 2019-02-27T17:37:54Z

I mean the fastest thing is to revert that commit; but it’d be worth spending some time first to figure out if there’s a way to keep it, and why it caused a failure.

jeffshaver · 2019-02-27T17:41:45Z

Just didn't know if we were going to try and put out a release before that. That commit is almost a year old. So that test will have been broken for quite some time. If we want to take the time and figure it out, than I understand that. Obviously I have a bit of bias with wanting this and my other PR to get in, so I can completely get off of TSLint on my project at work.

Should I try to revert that commit and see if tests break or whatever?

ljharb · 2019-02-27T17:58:15Z

It might not be the commit, but it might be the latest version of resolve. What happens if you peg it at ~1.9?

jeffshaver · 2019-02-27T22:27:01Z

@ljharb tried pinning 1.9.0 and 1.6.0. Didn't work :(

ljharb · 2019-02-27T23:50:33Z

@jeffshaver then that implies that that commit isn't what broke tests, given that the tests passed on #1069 - but rather, that something else in our dependency graph got updated around that time, and started breaking things.

jeffshaver · 2019-02-28T00:42:38Z

@ljharb, maybe I'll downgrade the deps one by one to see if I can figure it out. I tried a few

ljharb · 2019-02-28T01:06:55Z

Note that it also might be a transitive dep, that you can't touch via package.json. You may want to try temporarily including a lockfile, using npm 6.9's before time travelling support, using the date that tests last passed, and diff that against the date they first failed.

jeffshaver · 2019-02-28T02:36:36Z

@ljharb if we can pin the following packages and their transitive deps, then the test passes (ALL tests!):

eslint-import-resolver-node@^0.3.2
has@^1.0.1
[email protected]
resolve@^1.6.0

jeffshaver · 2019-02-28T02:48:34Z

I am having issues on how to get those all pinned (with a lockfile...). If i do an npm install and then an npm install --only=prod --before 04/06/2018, then all pass. but then a fresh npm install (from what should be the lockfile), it fails...

ljharb · 2019-02-28T04:21:55Z

ok - so let's try npm install --only=prod --before 04/06/2018 --package-lock --package-lock-only && mv package-lock.json lock-a.json and then npm install --only=prod --before 04/07/2018 --package-lock --package-lock-only && mv package-lock.json lock-b.json and then diff the two files? (replace the 7th with whatever date it starts breaking)

jeffshaver · 2019-03-01T00:05:04Z

difference between the 6th and before the 8th show these differences:

▶ diff lock-a.json lock-b.json
2435,2438c2435,2441
<       "version": "0.4.19",
<       "resolved": "https://registry.npmjs.org/iconv-lite/-/iconv-lite-0.4.19.tgz",
<       "integrity": "sha512-oTZqweIP51xaGPI4uPa56/Pri/480R+mo7SeU+YETByQNhDG55ycFyNLIgta9vXhILrxXDmF7ZGhqZIcuN0gJQ==",
<       "dev": true
---
>       "version": "0.4.21",
>       "resolved": "https://registry.npmjs.org/iconv-lite/-/iconv-lite-0.4.21.tgz",
>       "integrity": "sha512-En5V9za5mBt2oUA03WGD3TwDv0MKAruqsuxstbMUZaj9W9k/m1CV/9py3l0L5kw9Bln8fdHQmzHSYtvpvTLpKw==",
>       "dev": true,
>       "requires": {
>         "safer-buffer": "^2.1.0"
>       }
4125,4127c4128,4130
<       "version": "1.6.0",
<       "resolved": "https://registry.npmjs.org/resolve/-/resolve-1.6.0.tgz",
<       "integrity": "sha512-mw7JQNu5ExIkcw4LPih0owX/TZXjD/ZUF/ZQ/pDnkw3ZKhDcZZw5klmBlj6gVMwjQ3Pz5Jgu7F3d0jcDVuEWdw==",
---
>       "version": "1.7.0",
>       "resolved": "https://registry.npmjs.org/resolve/-/resolve-1.7.0.tgz",
>       "integrity": "sha512-QdgZ5bjR1WAlpLaO5yHepFvC+o3rCr6wpfE2tpJNMkXdulf2jKomQBdNRQITF3ZKHNlT71syG98yQP03gasgnA==",
4218a4222,4227
>     "safer-buffer": {
>       "version": "2.1.0",
>       "resolved": "https://registry.npmjs.org/safer-buffer/-/safer-buffer-2.1.0.tgz",
>       "integrity": "sha512-HQhCIIl7TrF1aa7d352EXG+xumPERvoIWxOqq2CagDId0FVGtlG/fuQ7kZT+wZ7ytyGiP3pnYUVni5otBzOVmA==",
>       "dev": true
>     },

ljharb · 2019-03-02T22:47:31Z

When I run npx npm@next install --before=2018-04-06 the test still fails. update: that's because the install fails :-/

ljharb · 2019-03-03T05:07:43Z

k, narrowed it down: a before date of 2019-02-05 passes, 2019-02-06 fails. A diff of the package-locks shows:

$ diff 5.json 6.json 
968,972d967
<     "builtin-modules": {
<       "version": "1.1.1",
<       "resolved": "https://registry.npmjs.org/builtin-modules/-/builtin-modules-1.1.1.tgz",
<       "integrity": "sha1-Jw8HbFpywC9bZaR9+Uxf46J4iS8="
<     },
3234,3241d3228
<     "is-builtin-module": {
<       "version": "1.0.0",
<       "resolved": "https://registry.npmjs.org/is-builtin-module/-/is-builtin-module-1.0.0.tgz",
<       "integrity": "sha1-VAVy0096wxGfj3bDDLwbHgN6/74=",
<       "requires": {
<         "builtin-modules": "^1.0.0"
<       }
<     },
4190,4192c4177,4179
<       "version": "2.4.2",
<       "resolved": "https://registry.npmjs.org/normalize-package-data/-/normalize-package-data-2.4.2.tgz",
<       "integrity": "sha512-YcMnjqeoUckXTPKZSAsPjUPLxH85XotbpqK3w4RyCwdFQSU5FxxBys8buehkSfg0j9fKvV1hn7O0+8reEgkAiw==",
---
>       "version": "2.5.0",
>       "resolved": "https://registry.npmjs.org/normalize-package-data/-/normalize-package-data-2.5.0.tgz",
>       "integrity": "sha512-/5CMN3T0R4XTj4DcGaexo+roZSdSFW/0AOOTROrjxzCG1wrWXEsGbRKevjlIL+ZDE4sZlJr5ED4YW0yqmkK+eA==",
4195c4182
<         "is-builtin-module": "^1.0.0",
---
>         "resolve": "^1.10.0",
8737,8739c8724,8726
<       "version": "3.7.0",
<       "resolved": "https://registry.npmjs.org/tsconfig-paths/-/tsconfig-paths-3.7.0.tgz",
<       "integrity": "sha512-7iE+Q/2E1lgvxD+c0Ot+GFFmgmfIjt/zCayyruXkXQ84BLT85gHXy0WSoQSiuFX9+d+keE/jiON7notV74ZY+A==",
---
>       "version": "3.8.0",
>       "resolved": "https://registry.npmjs.org/tsconfig-paths/-/tsconfig-paths-3.8.0.tgz",
>       "integrity": "sha512-zZEYFo4sjORK8W58ENkRn9s+HmQFkkwydDG7My5s/fnfr2YYCaiyXe/HBUcIgU8epEKOXwiahOO+KZYjiXlWyQ==",

Looks like it's normalize-package-date v2.5.0, but that suggests the test is badly written (because the only change there should be using resolve instead of is-builtin-module, which actually works properly). I'll look into it.

…n erstwhile transitive dep

ljharb · 2019-03-03T05:38:04Z

ha, turns out the tests were depending on builtin-modules being present; but now it no longer is. Fixed.

[Tests] fix a bunch of tests that were failing

b4bad0e

jeffshaver force-pushed the fix-tests branch from a477df1 to 193d786 Compare February 24, 2019 23:09

[Tests] use resolve, an actual dep, instead of builtin-modules, a…

158cd80

…n erstwhile transitive dep

ljharb force-pushed the fix-tests branch from 193d786 to 158cd80 Compare March 3, 2019 05:38

ljharb merged commit 083bb47 into import-js:master Mar 3, 2019

jeffshaver mentioned this pull request Mar 3, 2019

closes #1293. allows aliases that start with @ to be caught as internal #1294

Merged

ValeryVS mentioned this pull request Nov 11, 2019

import/order confused if some imported files are symlinks #1537

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix a bunch of tests that were failing #1295

fix a bunch of tests that were failing #1295

jeffshaver commented Feb 24, 2019

coveralls commented Feb 24, 2019 •

edited

Loading

ljharb commented Feb 25, 2019

jeffshaver commented Feb 25, 2019

jeffshaver commented Feb 25, 2019 •

edited

Loading

jeffshaver commented Feb 25, 2019 •

edited

Loading

jeffshaver commented Feb 26, 2019

ljharb commented Feb 26, 2019

jeffshaver commented Feb 26, 2019

ljharb commented Feb 26, 2019

jeffshaver commented Feb 26, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 28, 2019

ljharb commented Feb 28, 2019

jeffshaver commented Feb 28, 2019

jeffshaver commented Feb 28, 2019

ljharb commented Feb 28, 2019

jeffshaver commented Mar 1, 2019

ljharb commented Mar 2, 2019 •

edited

Loading

ljharb commented Mar 3, 2019

ljharb commented Mar 3, 2019 •

edited

Loading

fix a bunch of tests that were failing #1295

fix a bunch of tests that were failing #1295

Conversation

jeffshaver commented Feb 24, 2019

coveralls commented Feb 24, 2019 • edited Loading

ljharb commented Feb 25, 2019

jeffshaver commented Feb 25, 2019

jeffshaver commented Feb 25, 2019 • edited Loading

jeffshaver commented Feb 25, 2019 • edited Loading

jeffshaver commented Feb 26, 2019

ljharb commented Feb 26, 2019

jeffshaver commented Feb 26, 2019

ljharb commented Feb 26, 2019

jeffshaver commented Feb 26, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 27, 2019

ljharb commented Feb 27, 2019

jeffshaver commented Feb 28, 2019

ljharb commented Feb 28, 2019

jeffshaver commented Feb 28, 2019

jeffshaver commented Feb 28, 2019

ljharb commented Feb 28, 2019

jeffshaver commented Mar 1, 2019

ljharb commented Mar 2, 2019 • edited Loading

ljharb commented Mar 3, 2019

ljharb commented Mar 3, 2019 • edited Loading

coveralls commented Feb 24, 2019 •

edited

Loading

jeffshaver commented Feb 25, 2019 •

edited

Loading

jeffshaver commented Feb 25, 2019 •

edited

Loading

ljharb commented Mar 2, 2019 •

edited

Loading

ljharb commented Mar 3, 2019 •

edited

Loading